Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move CloudWatch rate limit to config #1673

Merged
merged 7 commits into from
Aug 31, 2016

Conversation

tmonk42
Copy link
Contributor

@tmonk42 tmonk42 commented Aug 26, 2016

Required for all PRs:

  • CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
  • Sign CLA (if not already signed)
  • README.md updated (if adding a new plugin)

Reference #1670

@sparrc
Copy link
Contributor

sparrc commented Aug 29, 2016

thanks @tmonk42, but you should also set 10 to be the default in the init() function.

@tmonk42
Copy link
Contributor Author

tmonk42 commented Aug 29, 2016

I dug through several other input plugins and found a common pattern of adding a setDefaults or setDefaultValues function inside of Gather(). I opted to follow this pattern rather than set it in init(). I hope that is acceptable.

@sparrc
Copy link
Contributor

sparrc commented Aug 30, 2016

sorry, that's not acceptable, please put it in the init() function. There's no point in calling setDefaults on every Gather when it can only be set once.

If there are other plugins doing that then they should be changed to use the init() method.

@tmonk42
Copy link
Contributor Author

tmonk42 commented Aug 30, 2016

https://github.com/influxdata/telegraf/blob/master/plugins/inputs/conntrack/conntrack.go#L40
https://github.com/influxdata/telegraf/blob/master/plugins/inputs/dns_query/dns_query.go#L80

Looks like the Ceph input plugin was updated to move to the init() function for defaults just a few hours ago. I'll update again :) thank you!

@sparrc
Copy link
Contributor

sparrc commented Aug 31, 2016

thanks @tmonk42!

@sparrc sparrc merged commit 2dc4728 into influxdata:master Aug 31, 2016
@tmonk42 tmonk42 deleted the cloudwatch_throttle_config branch August 31, 2016 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants